Skip to content

Migrate 9p file system to new backend - #1113

Open
Jay Bosamiya (Microsoft) (jaybosamiya-ms) wants to merge 4 commits into
mainfrom
jayb/ninep-migration
Open

Migrate 9p file system to new backend#1113
Jay Bosamiya (Microsoft) (jaybosamiya-ms) wants to merge 4 commits into
mainfrom
jayb/ninep-migration

Conversation

@jaybosamiya-ms

Copy link
Copy Markdown
Member

This PR switches our 9p filesystem to the new core file system design (see #887). Like prior migrations, it adds a new backend, migrates all old usages to a resolver-backed one to use the new backend, and then removes the old FileSystem object.

Additionally, as a drive-by-fix, the old read_dir's check reports every entry as a regular file (because it looked at e.typ rather than e.qid.typ); the new backend fixes this.

@jaybosamiya-ms

Copy link
Copy Markdown
Member Author

Similar to other migrations, it is easiest to review one commit at a time.

@wdcui

Copy link
Copy Markdown
Member

GPT-5.6 Sol: Requested changes—found 5 issues:

• High:  O_CREAT | O_EXCL | O_TRUNC  may truncate an existing file before returning  AlreadyExists  ( nine_p/mod.rs:417-425 ,  resolver.rs:526-530 ).
• High: Write-opening a directory can panic via  unimplemented!()  ( nine_p/mod.rs:355-358 ).
• Medium: Concurrent appenders can overwrite each other ( nine_p/mod.rs:500-513 ,  resolver.rs:661-671 ).
• Medium: Seeking beyond EOF is rejected, breaking sparse-file creation ( nine_p/mod.rs:486-488 ,  resolver.rs:716-720 ).
• Medium: A malformed 9P walk response can trigger an assertion panic ( nine_p/mod.rs:324-332 ).

}
// It means that the walk failed at the nwqid-th element
if new_len < chunk.len() {
// XXX: Per 9P2000.L the server does not establish `new_f` on a short walk, so not

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weiteng Chen (@CvvT) do you remember why we have a clunk here?

@wdcui Weidong Cui (wdcui) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks. Please take a look at the issues reported by GPT-5.6 sol.

@jaybosamiya-ms

Copy link
Copy Markdown
Member Author

Thanks Weidong, quick notes on the issues your agent raised:

  1. same as the one on a prior PR, I already have a TODO on this
  2. unimplemented!() panics are expected as "fixed in future PR"
  3. this is already tracked at the resolver
  4. this is already tracked in the resolver
  5. yes there is an assertion there, that is literally stating "the server is working as intended", surviving a malformed 9P server is not a thing we need to protect against, right? Either way would be outside the purview of this PR.

Note to myself: need to rebase after #1111 is merged before I merge

Base automatically changed from jayb/reduced-locking-on-iter to main August 12, 2026 01:01
@github-actions

Copy link
Copy Markdown

🤖 SemverChecks 🤖 ⚠️ Potential breaking API changes detected ⚠️

Click for details
--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/struct_missing.ron

Failed in:
  struct litebox::fs::nine_p::FileSystem, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/29132b2d2bd82ddaf2eafc3df584239b8388c626/litebox/src/fs/nine_p/mod.rs:271

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants